Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR addresses path-related warnings by replacing direct path resolution methods with FileUtil utility methods throughout the codebase. The changes aim to improve path handling consistency and likely address security or compatibility concerns related to path traversal.
- Replace direct
Path.resolve()calls withFileUtil.appendName() - Replace
new File(parent, child)constructors withFileUtil.appendName() - Add path traversal validation in file handling
- Update method signatures with null safety annotations
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| StudyImportContext.java | Replace path resolution with FileUtil.appendName |
| StudyPublishManager.java | Replace path resolution with FileUtil.appendName |
| PipelineServiceImpl.java | Replace path resolution with FileUtil.appendName |
| FileSystemAttachmentParent.java | Replace path resolution with FileUtil.appendName |
| FileContentServiceImpl.java | Add null annotations and replace path construction |
| ExperimentPipelineProvider.java | Replace path resolution with FileUtil.appendName |
| ExperimentController.java | Replace path resolution with FileUtil.appendName |
| FileType.java | Replace path resolution and File constructor with FileUtil methods |
| AbstractQueryImportAction.java | Refactor path handling with improved validation |
| LocalDirectory.java | Replace path resolution with FileUtil.appendName |
| AnalyzeForm.java | Add FileUtil import and replace path resolution |
| FileContentService.java | Add null annotation to method signature |
| AbstractFileXarSource.java | Add path traversal validation |
| FileAttachmentFile.java | Add null safety checks |
| AssayRunUploadForm.java | Add FileUtil import and replace File constructor |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| public File newFile(File parent, String basename) | ||
| { | ||
| return new File(parent, getName(parent, basename)); | ||
| return FileUtil.appendName(parent, getName(parent, basename)); |
There was a problem hiding this comment.
The return type is File but FileUtil.appendName(File, String) likely returns a Path or different type. This will cause a compilation error.
| return FileUtil.appendName(parent, getName(parent, basename)); | |
| return FileUtil.appendName(parent, getName(parent, basename)).toFile(); |
| { | ||
| // For local, the path may be several directories deep (since it matches the LK folder path), so we should create the directories for that path | ||
| fileRootPath = new File(parentRoot.toFile(), getRelativePath(c, firstOverride)).toPath(); | ||
| fileRootPath = FileUtil.appendPath(parentRoot.toFile(), Path.parse(getRelativePath(c, firstOverride))).toPath(); |
There was a problem hiding this comment.
This code converts Path to File, then back to Path unnecessarily. Should use FileUtil.appendPath directly with Path objects or use a consistent approach.
| fileRootPath = FileUtil.appendPath(parentRoot.toFile(), Path.parse(getRelativePath(c, firstOverride))).toPath(); | |
| fileRootPath = FileUtil.appendPath(parentRoot, Path.parse(getRelativePath(c, firstOverride))); |
| public FileAttachmentFile(FileLike file) | ||
| { | ||
| this(file.toNioPathForRead().toFile(), null); | ||
| this(file == null ? null : file.toNioPathForRead().toFile(), null); |
There was a problem hiding this comment.
We should probably prohibit null for the file. Lots of methods will NPE if _file is null.
| File f = new File(path); | ||
| if (!root.isUnderRoot(f)) | ||
| f = root.resolvePath(path); | ||
| Path relativePath = Path.parse(path); |
There was a problem hiding this comment.
I looked at the history and came to the same conclusion - this has been broken since 24.11. Let's get rid of both the relative and absolute variants here, but retain the webdav part above.
Rationale
Related Pull Requests
Changes